-
Notifications
You must be signed in to change notification settings - Fork 177
Add starting point for figures in StateGraph. #4682
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
These originate from figures created for Wolfram System Modeler, and are likely to need cleanup and improvement.
HansOlsson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They seem good, except for ShowCompositeStep that seems overloaded with information.
(8 different curves in one of several sub-plots!)
To me it makes sense that the default figure doesn't show everything - and if someone wants to understand the composite-step open up a different figure.
- I'm also unsure if "Output from initialStep" is a good description for
initialStep.active, (as there's also outPort) would "Active initialStep" be clearer (and also shorther)? - I notice that plotting wait-time together with active only works when wait-time is about 1s - so we might need to re-consider for different models, but it works for these.
- There's a bit of tendency of information overload in ShowExceptions as well, but I think it works.
|
Oh, and adding more textual description in the figures (as in the linked one) would also be good. |
|
@HansOlsson, your input is appreciated. Now we just need the library officer(s) to step in and propose or make the concrete changes. From the Wolfram side, we can use our experience with figures in general to confirm that proposed changes look sensible, but we are not the domain experts who would know best which variables to include in the plots, or what to write in the captions. |
Hmm... Checking for Modelica.StateGraph, it's @HansOlsson and @MartinOtter ! |
Great! In my opinion, having a caption should be a base requirement that all figures should live up to before a PR like this is accepted. How about writing down this sort of rule in https://github.com/modelica/ModelicaStandardLibrary/blob/master/Modelica/UsersGuide/Conventions.mo? With guidelines to follow, it should be easier to recognize when these PRs with the first round of figures are ready to be merged, and we would get more consistent looks across sub-libraries. |
| figures = { | ||
| Figure( | ||
| title = "Boolean signal in system", | ||
| identifier = "22111", | ||
| preferred = true, | ||
| plots = { | ||
| Plot( | ||
| curves = { | ||
| Curve(y = step0.active, legend = "Output from step0"), | ||
| Curve(y = transition1.t, legend = "transition1 wait time") | ||
| } | ||
| ), | ||
| Plot( | ||
| curves = { | ||
| Curve(y = step1.active, legend = "Output from step1") | ||
| } | ||
| ), | ||
| Plot( | ||
| curves = { | ||
| Curve(y = compositeStep.active, legend = "Output from entire compositeStep block = sum of initStep, step3, and exitStep"), | ||
| Curve(y = compositeStep.initStep.active, legend = "Output from initStep"), | ||
| Curve(y = compositeStep.step3.active, legend = "Output from step3"), | ||
| Curve(y = compositeStep.exitStep.active, legend = "Output from exitStep"), | ||
| Curve(y = compositeStep.transition3.t, legend = "transistion3 wait time"), | ||
| Curve(y = compositeStep.transition5.t, legend = "transition5 wait time"), | ||
| Curve(y = compositeStep.transition5.waitTime, legend = "transition5 target wait time"), | ||
| Curve(y = transition2.t, legend = "transition2 wait time") | ||
| } | ||
| ), | ||
| Plot( | ||
| curves = { | ||
| Curve(y = step6.active, legend = "Output from step6"), | ||
| Curve(y = transition7.condition, legend = "transition7 condition for firing status") | ||
| } | ||
| ) | ||
| } | ||
| ) | ||
| } | ||
| ), experiment(StopTime=15)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| figures = { | |
| Figure( | |
| title = "Boolean signal in system", | |
| identifier = "22111", | |
| preferred = true, | |
| plots = { | |
| Plot( | |
| curves = { | |
| Curve(y = step0.active, legend = "Output from step0"), | |
| Curve(y = transition1.t, legend = "transition1 wait time") | |
| } | |
| ), | |
| Plot( | |
| curves = { | |
| Curve(y = step1.active, legend = "Output from step1") | |
| } | |
| ), | |
| Plot( | |
| curves = { | |
| Curve(y = compositeStep.active, legend = "Output from entire compositeStep block = sum of initStep, step3, and exitStep"), | |
| Curve(y = compositeStep.initStep.active, legend = "Output from initStep"), | |
| Curve(y = compositeStep.step3.active, legend = "Output from step3"), | |
| Curve(y = compositeStep.exitStep.active, legend = "Output from exitStep"), | |
| Curve(y = compositeStep.transition3.t, legend = "transistion3 wait time"), | |
| Curve(y = compositeStep.transition5.t, legend = "transition5 wait time"), | |
| Curve(y = compositeStep.transition5.waitTime, legend = "transition5 target wait time"), | |
| Curve(y = transition2.t, legend = "transition2 wait time") | |
| } | |
| ), | |
| Plot( | |
| curves = { | |
| Curve(y = step6.active, legend = "Output from step6"), | |
| Curve(y = transition7.condition, legend = "transition7 condition for firing status") | |
| } | |
| ) | |
| } | |
| ) | |
| } | |
| ), experiment(StopTime=15)); | |
| figures = { | |
| Figure( | |
| title = "Cyclic activation of a parallel and alternative steps.", | |
| caption="This shows that steps can be in parallel (synchronized at the end).\nAdditionally it contains alternatives, that are selected based on priority.\nThe compositeStep shows how to build structure" | |
| identifier = "22111", | |
| preferred = true, | |
| plots = { | |
| Plot( | |
| curves = { | |
| Curve(y = step0.active, legend = "Active step0"), | |
| Curve(y = compositeStep.active, legend = "Active compositeStep - parallel to step1"), | |
| Curve(y = step1.active, legend = "Active step1"), | |
| Curve(y = step6.active, legend = "Active step6")}), | |
| Plot( | |
| curves={ | |
| Curve(y = compositeStep.active, legend = "Active compositeStep - where step3, step4, step4a are alternatives"), | |
| Curve(y = compositeStep.initStep.active, legend = "Active compositeStep.initStep"), | |
| Curve(y = compositeStep.step3.active, legend = "Active compositeStep.step3"), | |
| Curve(y = compositeStep.step4.active, legend = "Active compositeStep.step4"), | |
| Curve(y = compositeStep.step4a.active, legend = "Active compositeStep.step4a"), | |
| Curve(y = compositeStep.exitStep.active, legend = "Active compositeStep.exitStep") | |
| } | |
| ) | |
| } | |
| ) | |
| } | |
| ), | |
| experiment(StopTime=15)); |
As I understand it, this illustrates the main point of the example as:
- step1 and compositeStep are in parallel; so both are active at the same time (there's a synchronization when ending)
- inside compositeStep there are alternatives (step3, step4, step4a) - where only one is active.
The actual timings etc aren't the focus, so don't plot them.
(Should have a similar figure for ExecutionPaths.)
I believe the example would be clearer if different alternatives were selected for the different cycles.
| annotation ( | ||
| Documentation( | ||
| figures = { | ||
| Figure( | ||
| title = "Boolean signal in system", | ||
| identifier = "ef029", | ||
| preferred = true, | ||
| plots = { | ||
| Plot( | ||
| curves = { | ||
| Curve(y = initialStep.active, legend = "Output from initialStep"), | ||
| Curve(y = transition1.t, legend = "transition1 wait time") | ||
| } | ||
| ), | ||
| Plot( | ||
| curves = { | ||
| Curve(y = step.active, legend = "Output from step"), | ||
| Curve(y = transition2.t, legend = "transition2 wait time") | ||
| } | ||
| ) | ||
| } | ||
| ) | ||
| } | ||
| ), | ||
| experiment(StopTime=5.5)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| annotation ( | |
| Documentation( | |
| figures = { | |
| Figure( | |
| title = "Boolean signal in system", | |
| identifier = "ef029", | |
| preferred = true, | |
| plots = { | |
| Plot( | |
| curves = { | |
| Curve(y = initialStep.active, legend = "Output from initialStep"), | |
| Curve(y = transition1.t, legend = "transition1 wait time") | |
| } | |
| ), | |
| Plot( | |
| curves = { | |
| Curve(y = step.active, legend = "Output from step"), | |
| Curve(y = transition2.t, legend = "transition2 wait time") | |
| } | |
| ) | |
| } | |
| ) | |
| } | |
| ), | |
| experiment(StopTime=5.5)); | |
| annotation ( | |
| Documentation( | |
| figures = { | |
| Figure( | |
| title = "Cyclic activation of the states", | |
| identifier = "ef029", | |
| preferred = true, | |
| caption="Shows how the system cycles from initialStep to Step, and then repeated.\nThe time before transition is determined by the waitTime (1s) of the transitions.", | |
| plots = { | |
| Plot( | |
| curves = { | |
| Curve(y = initialStep.active, legend = "Active initialStep"), | |
| Curve(y = step.active, legend = "Active step")}), | |
| Plot( | |
| curves = { | |
| Curve(y = transition1.t, legend = "transition1 time waiting"), | |
| Curve(y = transition2.waitTime, legend = "transition1 waitTime parameter")})})}), | |
| experiment(StopTime=5.5)); |
After some thinking I would prefer to re-order, and re-name the plots like that, except I'm not happy with the "waitTime parameter" part.
The reasons are:
- Having activations in one plot may make it clearer that just one state is active.
- Calling it "Active initialStep" was as indicated clearer to me
- Given that there is a parameter "waitTime" (the time until transition triggers) plotting another variable as "wait time" was a bit confusing to me, so I renamed it to "time waiting".
- One could add another sub-plot for transition2, but I don't think it is needed.
At first I had the existing two transition*.t in one plot - but it was harder to understand for me - especially if one plotted waitTime as well.
|
I didn't update the plots for Modelica.StateGraph.Examples.ShowExceptions yet, as I think it needs improvement. |
|
I would propose to not add a figure for ShowExceptions at the current state, but instead add it together with the other improvements of that example - and then make sure that the Figure is actually understandable; see #4722 Note that the original figures were sort of ok, but they didn't really illustrate the examples that well, partially since the models don't do a good job of that either. I don't know why no-one have noticed earlier, but it seems that adding figures helps in several ways. |
These originate from figures created for Wolfram System Modeler, and are likely to need cleanup and improvement.
Our one existing figure in MSL is probably a good style to follow:
ModelicaStandardLibrary/Modelica/Blocks/package.mo
Lines 159 to 180 in 3f72865
Creating as Draft to indicate that library officer(s) probably want/should improve them before merging.